fixes several bugs on GNRC network stack #4447

Yonezawa-T2 · 2015-12-09T10:07:32Z

This PR fixes several bugs in GNRC network stack.

fixed buffer overrun results from off-by-one error
fixed crash when there are no 6LoWPAN routers
fixed crash when both 6LoWPAN network and convensional IPv6 network are enabled
fixed infinite loop in managing network prefixes
fixed crash in debug output code
fixed crash by cancelling timer no longer needed
adds extra debug outputs for FIB

See commit messages for details.

This PR may or may not be broken by #3622. I will investigate later.

Tested on OS X with modifications of #4443, #4444, #4445, and #4446.

OlegHahm · 2015-12-09T10:11:12Z

Wow, I haven't looked at the changes in detail, but I'd like already thank you for sharing your findings and providing all these fixes!

cgundogan · 2015-12-09T10:32:42Z

Thank you @Yonezawa-T2 ! I will take a look at your proposed fixes.

cgundogan · 2015-12-09T10:36:16Z

sys/net/gnrc/network_layer/ipv6/nc/gnrc_ipv6_nc.c

 #endif
-#if defined(MODULE_GNRC_NDP_ROUTER) || defined(MODULE_GNRC_SIXLOWPAN_ND_BORDER_ROUTER)
+#if defined(MODULE_GNRC_NDP_ROUTER) || defined(MODULE_GNRC_SIXLOWPAN_ND_BORDER_ROUTER) || defined(MODULE_GNRC_SIXLOWPAN_ND_ROUTER)


rtr_adv_timer is not defined in gnrc_ipv6_nc_t for MODULE_GNRC_SIXLOWPAN_ND_ROUTER

Yonezawa-T2 · 2015-12-09T10:43:37Z

@cgundogan fixed.

cgundogan · 2015-12-09T11:06:19Z

sys/net/gnrc/network_layer/ipv6/nc/gnrc_ipv6_nc.c

+
+        gnrc_ipv6_netif_t *if_entry = gnrc_ipv6_netif_get(iface);
+
+        if (if_entry != NULL &&


could you put each condition in parenthesis?

cgundogan · 2015-12-09T15:03:36Z

@Yonezawa-T2 can you also add a mutex_unlock() in https://github.com/RIOT-OS/RIOT/blob/master/sys/net/gnrc/network_layer/ndp/internal/gnrc_ndp_internal.c#L472 and following return statements?

While your fix for the infinite prefix loop works for me, it is still possible to run into the same problem if the pktbuf is full and returns without unlocking the mutex.

It would block here: https://github.com/RIOT-OS/RIOT/blob/master/sys/net/gnrc/network_layer/ipv6/netif/gnrc_ipv6_netif.c#L133

Yonezawa-T2 · 2015-12-10T02:50:29Z

Updated.

miri64 · 2015-12-18T18:02:47Z

sys/net/gnrc/network_layer/ipv6/nc/gnrc_ipv6_nc.c

+        gnrc_ipv6_netif_t *if_entry = gnrc_ipv6_netif_get(iface);
+
+        if ((if_entry != NULL) &&
+              (if_entry->rtr_adv_msg.content.ptr == (char *) entry)) {


indentation (should however fit into 100 cols of the first line).

miri64 · 2015-12-18T18:16:07Z

Sorry for the late review, this PR wasn't on my radar somehow. Thank you for your contributions, but while most fixes you offer are legitimate, there are some that unnecessarily might make the code size bigger or assume things that might not be true in certain situations.

Yonezawa-T2 · 2015-12-21T04:07:08Z

All my fixes are based on concrete problems I met while running my application (#4448). Those fixes are necessary unless there are better solutions.

kaspar030 · 2015-12-21T10:52:50Z

@Yonezawa-T2 Again, thanks a lot for these fixes! For the next time, experience shows that reviewing is a lot faster when PR's are small, so PR'ing even seemingly trivial commits by themselves is usually a good idea.

cgundogan · 2015-12-21T18:07:47Z

@Yonezawa-T2, could you create a new PR and cherry-pick the following commits:
Yonezawa-T2@18c0720
Yonezawa-T2@0981896
Yonezawa-T2@4466bab
Yonezawa-T2@9e5c188
Yonezawa-T2@a87d36a

The above commits are those that I would ACK immediately.

cgundogan · 2016-01-04T17:57:46Z

@Yonezawa-T2, could you rebase onto current master?

Yonezawa-T2 · 2016-01-06T00:30:05Z

Rebased.

miri64 · 2016-01-06T14:17:31Z

sys/include/net/gnrc/ipv6/netif.h

@@ -481,6 +481,7 @@ ipv6_addr_t *gnrc_ipv6_netif_find_addr(kernel_pid_t pid, const ipv6_addr_t *addr
 * @param[out] out      The reference to the found address on the interface.
 *                      Must be a pointer to NULL on calling and may stay
 *                      unchanged if no match can be found.
+ *                      Must not be NULL.


Can you add this as a @pre tag instead?

miri64 · 2016-01-06T14:21:16Z

Apart from my last comment I'm fine with the changes. Please squash immediately, maybe there is a change to squeeze this one into the release, too.

When `gnrc_ndp_node_next_hop_l2addr` cannot resolve L2 address, it creates a temporary neighbor cache entry with interface `KERNEL_PID_UNDEF` (unless the interface is already known) to send a neighbor solicitation. When another packet directed to the same address is going to sent before receiving a neighbor advertisement, `gnrc_sixlowpan_nd_next_hop_l2addr` gets the temporary neighbor cache entry and calls `gnrc_ipv6_netif_get` with `KERNEL_PID_UNDEF`, resulting get a `NULL`. We must check `NULL` before dereference it. FYI, both `gnrc_ndp_node_next_hop_l2addr` and `gnrc_sixlowpan_nd_next_hop_l2addr` are enabled when `gnrc_sixlowpan_border_router_default` module is enabled with `GNRC_NETIF_NUMOF` is greater than 1: gnrc_sixlowpan_border_router_default → gnrc_ipv6_router_default → gnrc_ndp_router (if GNRC_NETIF_NUMOF > 1) → gnrc_ndp_node → gnrc_ndp_node_next_hop_l2addr is called from _next_hop_l2addr gnrc_sixlowpan_border_router_default → gnrc_sixlowpan_nd_border_router → gnrc_sixlowpan_nd_router → gnrc_sixlowpan_nd → gnrc_sixlowpan_nd_next_hop_l2addr is called from _next_hop_l2addr

When ENABLE_DEBUG is 1, `out` is dereferenced unconditionally, but `_parse_options` in `gnrc_rpl_control_messages.c` calls it with NULL. Clarified `out` must not NULL and fixed `_parse_options`.

…move`.

Yonezawa-T2 · 2016-01-07T01:16:42Z

Added @pre tag and squashed commits. Thanks for reviewing. The commits are much sophisticated.

miri64 · 2016-01-07T01:20:51Z

ACK and go as soon as travis is happy. Please provide a backport to 2015.12-branch.

fixes several bugs on GNRC network stack

OlegHahm assigned cgundogan Dec 9, 2015

OlegHahm added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: network Area: Networking NSTF Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch labels Dec 9, 2015

OlegHahm added this to the Release 2015.12 milestone Dec 9, 2015

Yonezawa-T2 mentioned this pull request Dec 9, 2015

examples: add example of border router with XBee on native platform. #4448

Closed

cgundogan reviewed Dec 9, 2015
View reviewed changes

miri64 reviewed Dec 18, 2015
View reviewed changes

This was referenced Dec 22, 2015

sixlowpan: fixed infinite loop #4531

Merged

gnrc_sixlowpan_nd: fixed crash when there are no routers #4532

Merged

gnrc_ndp_internal: add missing unlock when the pktbuf is full #4533

Merged

Yonezawa-T2 mentioned this pull request Dec 22, 2015

gnrc_ipv6_netif: fixed buffer overrun #4534

Merged

OlegHahm added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Dec 22, 2015

Yonezawa-T2 force-pushed the fix_gnrc_bugs branch from bec3774 to 48375f0 Compare January 6, 2016 00:29

miri64 reviewed Jan 6, 2016
View reviewed changes

Yonezawa-T2 added 4 commits January 7, 2016 10:02

gnrc_ipv6_netif: fixed crash when ENABLE_DEBUG is 1.

542e045

When ENABLE_DEBUG is 1, `out` is dereferenced unconditionally, but `_parse_options` in `gnrc_rpl_control_messages.c` calls it with NULL. Clarified `out` must not NULL and fixed `_parse_options`.

nc: cancel timer set by gnrc_ndp_rtr_sol_handle in `gnrc_ipv6_nc_re…

c9c83c7

…move`.

fib: add debug outputs

cf35763

Yonezawa-T2 force-pushed the fix_gnrc_bugs branch from 48375f0 to cf35763 Compare January 7, 2016 01:14

miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable labels Jan 7, 2016

Yonezawa-T2 mentioned this pull request Jan 7, 2016

fixes several bugs on GNRC network stack #4604

Merged

miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jan 7, 2016

miri64 added a commit that referenced this pull request Jan 7, 2016

Merge pull request #4447 from Yonezawa-T2/fix_gnrc_bugs

94e4a08

fixes several bugs on GNRC network stack

miri64 merged commit 94e4a08 into RIOT-OS:master Jan 7, 2016

miri64 removed the Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch label Jan 7, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixes several bugs on GNRC network stack #4447

fixes several bugs on GNRC network stack #4447

Yonezawa-T2 commented Dec 9, 2015

OlegHahm commented Dec 9, 2015

cgundogan commented Dec 9, 2015

cgundogan Dec 9, 2015

Yonezawa-T2 commented Dec 9, 2015

cgundogan Dec 9, 2015

cgundogan commented Dec 9, 2015

Yonezawa-T2 commented Dec 10, 2015

miri64 Dec 18, 2015

miri64 commented Dec 18, 2015

Yonezawa-T2 commented Dec 21, 2015

kaspar030 commented Dec 21, 2015

cgundogan commented Dec 21, 2015

cgundogan commented Jan 4, 2016

Yonezawa-T2 commented Jan 6, 2016

miri64 Jan 6, 2016

miri64 commented Jan 6, 2016

Yonezawa-T2 commented Jan 7, 2016

miri64 commented Jan 7, 2016


		gnrc_ipv6_netif_t *if_entry = gnrc_ipv6_netif_get(iface);

		if (if_entry != NULL &&

fixes several bugs on GNRC network stack #4447

fixes several bugs on GNRC network stack #4447

Conversation

Yonezawa-T2 commented Dec 9, 2015

OlegHahm commented Dec 9, 2015

cgundogan commented Dec 9, 2015

cgundogan Dec 9, 2015

Choose a reason for hiding this comment

Yonezawa-T2 commented Dec 9, 2015

cgundogan Dec 9, 2015

Choose a reason for hiding this comment

cgundogan commented Dec 9, 2015

Yonezawa-T2 commented Dec 10, 2015

miri64 Dec 18, 2015

Choose a reason for hiding this comment

miri64 commented Dec 18, 2015

Yonezawa-T2 commented Dec 21, 2015

kaspar030 commented Dec 21, 2015

cgundogan commented Dec 21, 2015

cgundogan commented Jan 4, 2016

Yonezawa-T2 commented Jan 6, 2016

miri64 Jan 6, 2016

Choose a reason for hiding this comment

miri64 commented Jan 6, 2016

Yonezawa-T2 commented Jan 7, 2016

miri64 commented Jan 7, 2016